[WFCORE-6750] Analysis document for the Unstable API Annotation scanning #569
Conversation
This fixes wildfly#568 Signed-off-by: Jeff Mesnil <[email protected]>
| *** It also contains a simple configuration mechanism to narrow down which classpath entries to index (e.g when scanning for `io.smallrye.common.annotation.Experimental` we only check the SmallRye libraries on the classpath) | ||
| * In WildFly Core | ||
| ** deployment unit processors will be added to: | ||
| *** Load the indices listed in the `content/index.html` file in the `{indexModule}` module. |
There was a problem hiding this comment.
s/index.html/index.txt
| ** The deployment unit processors will use the WildFly classloading rules to make sure that in the case of nested archives, that we only scan each class once. E.g. if a .war contains a `WEB-INF/lib/my-library.jar`, classes in `WEB-INF/lib/my-library.jar` will only be s scanned as part of the `WEB-INF/lib/my-library.jar` resource root scan, and not for the parent .war. | ||
| ** Once scanned, usage of annotated API elements may be reported depending on configuration in the `core-management` subsystem: | ||
| *** A new child resource (currently called `service=unstable-api-annotations`) is added with a `level` attribute to configure the reporting | ||
| **** Since this is a resource/feature with a stability level of preview, if the resource is not present, no reporting is done. |
There was a problem hiding this comment.
s/no reporting is done/no scanning or reporting is done/g
| ** User provided modules | ||
| ** Classes provided by the user via the management model with the typical `module`/`class` attribute pairs. | ||
| * No attempt is made to not add the indices if the server is provisioned a a higher stability level than 'preview'. In other words, if the feature packs contain the indices, they will always end up in the server. | ||
| * Since this is a `preview` feature, and the model version is the same as the original with the added `preview` qualifier, model transformation is not needed. |
There was a problem hiding this comment.
There should be a layer for this. Not having it now can be a 'Non-Requirement' + 'Future Work'.
I think the layer is a requirement for moving to Community stability.
When the layer is created we have to decided if this requires opt-in or opt-out when provisioning with layers. Going to opt-in would be a breaking change vs this initial "always there" approach, but OTOH this is the kind of "I lived without this" functionality that is better handled as an opt-in.
| * Promote this feature to Community or higher. | ||
| ** This would allow us to enable it out of the box by default, making it more useful. | ||
| * Identify all annotations indicating unstable API used by WildFly itself with the help of component leads. | ||
| ** Scan for these annotations during feature pack builds, and make them available in a provisioned server. |
There was a problem hiding this comment.
What would be the goal of this?
There was a problem hiding this comment.
To index similar annotations from other libraries than the current SmallRye and Hibernate families. E.g. I think JGroups has something. Apache might have something etc. i.e. get better coverage
There was a problem hiding this comment.
My question here is more about why we make them available in the provisioned server. But probably I misinterpreted the parent point.
If your meaning in the parent point is just to include more annotations from other components and check for their use in deployment code, I understand. If you're talking about scanning WF code for our own use of things, I understand why we'd do that, I'm just not clear why we'd want to write the result in the provisioned server.
In any case I'm not asking for any change in the document; this is just a discussion of what you had in mind.
| * In WildFly Core | ||
| ** deployment unit processors will be added to: | ||
| *** Load the indices listed in the `content/index.html` file in the `{indexModule}` module. | ||
| *** Use the wildfly/unstable-api-annotation-utils library to scan the bytecode of every class in the user's deployed application and cross-reference against the loaded indices. |
There was a problem hiding this comment.
This should be provided by a module that is an optional dependency of whatever uses this. That module has property jboss-stability-level=preview.
That likely means the index files should be in a different module from this lib, one that's at default stability, so the plugin execution that adds them doesn't have to know whether the FP build is configured to allow preview stability modules in the FP.
There was a problem hiding this comment.
I misspoke -- it's jboss.stability=preview, for example
For a brief discussion of this see 'JBoss Modules modules stability level' in https://docs.wildfly.org/wildfly-proposals/wf-galleon/WFLY-19021-Stability_In_Provisioning.html#requirements-at-feature-pack-build-time
| ** in user modules | ||
| ** in classes referenced via the `module`/`class` attribute pairs in the management model | ||
| * Add a deployment marker to force enable/disable the runtime scanning independent of the subsystem setting | ||
| ** This could possibly be enhanced with a filter so a user could say to ignore usage of constructs annotated with `org.hibernate.Incubating` (or perhaps an expression to ignore classes matching the expression), while reporting usage of everything else (e.g. constructs annotated with `io.smallrye.common.annotation.Experimental`). |
There was a problem hiding this comment.
If we're going to allow something like this, which involves the user identifying annotations of interest, then having that config do a mapping to a stability level seems good. Then how we treat the presence of the thing depends on the server's stability level.
If we found something that wasn't configured, that would mean 'not allowed' (so log it or error). IOW, not configured != experimental. Someone running a --stability=experimental server can still reject APIs.
There was a problem hiding this comment.
I consider this more brainstorming, than a "must" have. I've updated the start of the future work section to say we will decide the scope when going to community level, and that these points are ideas
There was a problem hiding this comment.
Great; that was my intent. :)
| WildFly: | ||
|
|
||
| * Since we have done the main testing of the mechanism in WildFly Core, we will not test that here | ||
| * A test will be added to `testsuite/integration/basic` which runs in an execution provisioning both `wildfly-ee-feature-pack` and `wildfly-galleon-pack`. It will test |
There was a problem hiding this comment.
This needs to be in testsuite/integration/microprofile (to be renamed 'expansion').
Use of wildfly-galleon-pack is not allowed in testsuite/integration/basic.
There was a problem hiding this comment.
Also s/wildfly-ee-feature-pack/wildfly-ee-galleon-pack
or, my minor preference
"both the 'wildfly-ee' and 'wildfly' feature packs."
There was a problem hiding this comment.
Good point, it will simplify the test since in basic there were two cases
- the full zip which tests everything
- the galleon provisioned server which don't have the wildfly FP
| Generally a feature should have documentation as part of the PR to wildfly master, or as a follow up PR if the feature is in wildfly-core. In some cases though the documentation belongs more in a component, or does not need any documentation. Indicate which of these will happen. | ||
| //// | ||
|
|
||
| Community documentations will be added as part of the PR to WildFly full. This PR will also contain the parts of the tests which live in WildFly, as mentioned in the previous section |
There was a problem hiding this comment.
We need to beef this bit up a lot. I think not implementing everything by the 33 feature freeze is ok, but we should think about things like user journeys.
There was a problem hiding this comment.
Actually I think what you did is ok for Preview. A blog would be good though, if not now then as part of moving to Community.
Human readable names of feature packs Point out that we are using zipped index files Scan the module resources rather than using a shared index.txt
| Community documentations will be added as part of the PR to WildFly full. This PR will also contain the parts of the tests which live in WildFly, as mentioned in the previous section | ||
|
|
||
| == Release Note Content | ||
| The core-management subsystem now allows you to enable scanning of your deployments for usage of classes/methods in the SmallRye and Hibernate libraries annotated with `org.hibernate.Incubating` and `io.smallrye.common.annotation.Experimental`. These annotations indicate that those API elements are likely to change at any time |
https://issues.redhat.com/browse/WFCORE-6750 (the runtime scanner)
https://issues.redhat.com/browse/WFLY-19152 (creating the indices in the feature pack build)